Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Drop annotations with NaN onset in EEGLAB raw files #12484

Merged
merged 10 commits into from
Mar 13, 2024

Conversation

withmywoessner
Copy link
Contributor

@withmywoessner withmywoessner commented Mar 5, 2024

Reference issue

Fixes #12425.

What does this implement/fix?

Fix bug where MNE cannot handle EEGLAB events with NaN annotations

Additional information

Rationale for decision from EEGLAB documentation:
image
https://eeglab.org/tutorials/04_Import/Importing_Event_Epoch_Info.html

@withmywoessner withmywoessner changed the title [WIP] Drop annotations with NaN onset in EEGLAB raw files [WIP] [BUG] Drop annotations with NaN onset in EEGLAB raw files Mar 5, 2024
@withmywoessner withmywoessner marked this pull request as ready for review March 5, 2024 19:45
@withmywoessner
Copy link
Contributor Author

withmywoessner commented Mar 5, 2024

@cbrnr Should be fixed now! Should I add that data to the testing dataset?

@withmywoessner withmywoessner changed the title [WIP] [BUG] Drop annotations with NaN onset in EEGLAB raw files [BUG] Drop annotations with NaN onset in EEGLAB raw files Mar 6, 2024
@cbrnr
Copy link
Contributor

cbrnr commented Mar 8, 2024

I would not add a new dataset, but can you add a test with some artificially generated markers that demonstrate the problem (and verify the fix)?

@withmywoessner
Copy link
Contributor Author

withmywoessner commented Mar 11, 2024

I would not add a new dataset, but can you add a test with some artificially generated markers that demonstrate the problem (and verify the fix)?

Would I have to make a temp file @cbrnr? Because from my understanding, I would have to read a MATLAB .set file then edit the annotations. Then I would need to read the file again to check to see if it drops the bad annotations. Can you think of another way?

EDIT: I think I can just import _read_annotations_eeglab directly

EDIT2: I looked and _read_annotations has some other functions that it is reliant on so I don't think I can just use that to read the events I get from io.loadmat(raw_fname_mat,...)

EDIT3: Yeah, since MATLAB files are stored as a binary and the _read_annotations expects a MATLAB file. I think I would have to read the MATLAB file, then edit the annotations, then re-export it as a binary.

@withmywoessner withmywoessner marked this pull request as draft March 11, 2024 23:57
@cbrnr
Copy link
Contributor

cbrnr commented Mar 12, 2024

Hm, yes, because the fix is in the reader it's probably easiest if you add a small(!) test file then.

@larsoner
Copy link
Member

EDIT3: Yeah, since MATLAB files are stored as a binary and the _read_annotations expects a MATLAB file. I think I would have to read the MATLAB file, then edit the annotations, then re-export it as a binary.

Can you try this and see if it's easy? If it's old format then scipy.io functions can be used. If it's new format then h5py could perhaps be used directly just to change one or two entries and save to a tmpdir?

@withmywoessner
Copy link
Contributor Author

I can try @larsoner should I just use the tempfilelibrary?

@larsoner
Copy link
Member

No in tests typically you use the pytest test fixture def test_something(tmp_path): and write for example in the test with open(tmp_path / "my_file.txt', 'w'): or similar.

@withmywoessner withmywoessner marked this pull request as ready for review March 12, 2024 16:36
@withmywoessner
Copy link
Contributor Author

Should be good now @larsoner and @cbrnr. Thanks!

@withmywoessner
Copy link
Contributor Author

There is an error related to testing. Not sure what the problem is. When I ran it locally it passed:
image

@larsoner
Copy link
Member

That's a weird error -- I put a little fix in #12491 so ignore it for now and we'll merge that quickly and merge main into your branch afterward to make sure everything is green

@larsoner
Copy link
Member

Just a couple of minor things, committed and marking for merge-when-green, thanks @withmywoessner !

@larsoner larsoner enabled auto-merge (squash) March 13, 2024 13:33
@larsoner
Copy link
Member

Pip-pre is unrelated, merging!

@larsoner larsoner disabled auto-merge March 13, 2024 15:31
@larsoner larsoner merged commit b906b78 into mne-tools:main Mar 13, 2024
28 of 30 checks passed
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
@withmywoessner withmywoessner deleted the EEGLAB_anno_bug branch March 25, 2024 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotation-related error reading EEGLAB file
3 participants